Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update the CONCAT scalar function to support Utf8View #12224

Merged
merged 18 commits into from
Sep 3, 2024

Conversation

devanbenz
Copy link
Contributor

@devanbenz devanbenz commented Aug 29, 2024

Which issue does this PR close?

Closes #11836

Rationale for this change

Explained within ticket #11836

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added sqllogictest SQL Logic Tests (.slt) functions labels Aug 29, 2024
@devanbenz devanbenz marked this pull request as ready for review August 29, 2024 03:01
Copy link
Contributor

@tshauck tshauck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one minor comment regarding LargeUtf8, but overall LGTM

@@ -46,7 +45,7 @@ impl ConcatFunc {
pub fn new() -> Self {
use DataType::*;
Self {
signature: Signature::variadic(vec![Utf8], Volatility::Immutable),
signature: Signature::variadic(vec![Utf8, Utf8View], Volatility::Immutable),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps as a follow on PR we can expand this to also support LargeUtf8s... though it looks like maybe they are from the tests? Does this just need to be updated and it'll work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's casting to Utf8 from LargeUtf8? Not sure. I can follow up and modify this code to include that though it should be relatively quick.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@tshauck tshauck Aug 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, should've been clearer, to your point, I think it works but causes a cast to utf8.

> CREATE TABLE ttt AS SELECT arrow_cast('T', 'LargeUtf8') AS a;
0 row(s) fetched. 
Elapsed 0.012 seconds.

> SELECt ttt.a FROM ttt;
+---+
| a |
+---+
| T |
+---+
1 row(s) fetched. 
Elapsed 0.007 seconds.

> EXPLAIN SELECt ttt.a FROM ttt;
+---------------+-----------------------------------------------+
| plan_type     | plan                                          |
+---------------+-----------------------------------------------+
| logical_plan  | TableScan: ttt projection=[a]                 |
| physical_plan | MemoryExec: partitions=1, partition_sizes=[1] |
|               |                                               |
+---------------+-----------------------------------------------+
2 row(s) fetched. 
Elapsed 0.007 seconds.

> EXPLAIN SELECt concat(ttt.a, ttt.a) FROM ttt;
+---------------+--------------------------------------------------------------------------------------------+
| plan_type     | plan                                                                                       |
+---------------+--------------------------------------------------------------------------------------------+
| logical_plan  | Projection: concat(__common_expr_1 AS ttt.a, __common_expr_1 AS ttt.a)                     |
|               |   Projection: CAST(ttt.a AS Utf8) AS __common_expr_1                                       |
|               |     TableScan: ttt projection=[a]                                                          |
| physical_plan | ProjectionExec: expr=[concat(__common_expr_1@0, __common_expr_1@0) as concat(ttt.a,ttt.a)] |
|               |   ProjectionExec: expr=[CAST(a@0 AS Utf8) as __common_expr_1]                              |
|               |     MemoryExec: partitions=1, partition_sizes=[1]                                          |
|               |                                                                                            |
+---------------+--------------------------------------------------------------------------------------------+

I think if you were to concat two largeutf8s together, you should get that same type as the return?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct - I will need to modify the input/return values for this. I'll add an additional test for LargeUtf8 and fix in this PR tonight. It shouldn't take long, I think I have a path forward to change that and it should be relatively painless.

Copy link
Contributor Author

@devanbenz devanbenz Aug 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correction** I fixed it now it was really quick resolution lol

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

> SELECT ttt.a from ttt;
+---+
| a |
+---+
| T |
+---+
1 row(s) fetched.
Elapsed 0.009 seconds.

> EXPLAIN SELECT ttt.a from ttt;
+---------------+-----------------------------------------------+
| plan_type     | plan                                          |
+---------------+-----------------------------------------------+
| logical_plan  | TableScan: ttt projection=[a]                 |
| physical_plan | MemoryExec: partitions=1, partition_sizes=[1] |
|               |                                               |
+---------------+-----------------------------------------------+
2 row(s) fetched.
Elapsed 0.007 seconds.

> EXPLAIN SELECT concat(ttt.a, ttt.a) from ttt;
+---------------+----------------------------------------------------------------+
| plan_type     | plan                                                           |
+---------------+----------------------------------------------------------------+
| logical_plan  | Projection: concat(ttt.a, ttt.a)                               |
|               |   TableScan: ttt projection=[a]                                |
| physical_plan | ProjectionExec: expr=[concat(a@0, a@0) as concat(ttt.a,ttt.a)] |
|               |   MemoryExec: partitions=1, partition_sizes=[1]                |
|               |                                                                |
+---------------+----------------------------------------------------------------+
2 row(s) fetched.
Elapsed 0.009 seconds.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it's erroring on cast to generic string array. I need to modify 😆 whoops still not fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's resolved 👍

@@ -776,7 +776,7 @@ EXPLAIN SELECT
FROM test;
----
logical_plan
01)Projection: concat(CAST(test.column1_utf8view AS Utf8), CAST(test.column2_utf8view AS Utf8)) AS c
01)Projection: concat(test.column1_utf8view, test.column2_utf8view) AS c
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💪

datafusion/functions/src/string/concat.rs Outdated Show resolved Hide resolved
Comment on lines 71 to 75
Ok(match &arg_types[0] {
Utf8View => Utf8View,
LargeUtf8 => LargeUtf8,
_ => Utf8,
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the logic seems to assume all arguments are of the same type?

also, why not always return Utf8?
the code performing actual concatenation seems to be always the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah @findepi I think the logic is "Whatever the first argument type is the output should be of that type" so if the received values were: Utf8, Utf8View the output would be Utf8. I'm taking the logic from other UDFs and applying it here. It may not be the best way of doing this though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the logic is "Whatever the first argument type is the output should be of that type

i understand this is what's implemented. but not sure why it is so.
what's the exact benefit of presenting the data as string view, if we computed the exact string anyway, and we technically don't need to represent it as a string view?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay. So what you're saying is that here: https://github.com/apache/datafusion/pull/12224/files#diff-71970189679c6dd5b3b677bb21603234b488e68d1601be9c4d400d40e430a909R204 I'm building a Utf8 string anyways?

So I suspect I should change that bit of code to use a StringViewArrayBuilder?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that's my intuition
except that i would keep building StringArray and just declare return type as Utf8 always

from the issue #11836

Currently, a call to CONCAT with a Utf8View datatypes induces a cast. After the change that fixes this issue, it should not.

this is about inputs to the function, not the return type

Side note:
String view could be an interesting return type if we wanted to optimize for single non-null string view input and let it pass-through; but the code doesn't do this today, not sure it's worth implementing for this edge case and it should be independent of arguments order, ie not tied to the first input's type.
end of side note.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@findepi what about for LargeUtf8? I suspect that if a LargeUtf8 is the input then the output should also be that since its an i64 datatype vs the i32 datatype for Utf8?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't know the exact rules for how we handle LargeUtf8.
i focused on the string view portion of the PR. from "string views perspective", LargeUtf8 is non-issue, so IMO it's fine not to change the return type with respect to LargeUtf8 in this PR. but i agree that we probably should return LargeUtf8 when any input is LargeUtf8 (pr what exactly the logic should be).

in fact, what does the binary concat operator do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@findepi when you say binary concat operator are you talking about || as an operator?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when you say binary concat operator are you talking about || as an operator?

yes

the logic seems to assume all arguments are of the same type?

also, why not always return Utf8? the code performing actual concatenation seems to be always the same.

the question still holds (why exactly we bias towards the first param type), but i am no longer convinced about my suggestion to use Utf8 always.

i think we should "just" make sure concat(a, b, c) is type-equivalent to a || b || c.
the ||'s logic apparently is

  • if any of the operands are Utf8View, the result is Utf8View
  • else, if any of the operands are LargeUtf8, the result is LargeUtf8

cc @alamb

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good @findepi i like that logic. I can adjust to make it so.

@@ -418,6 +451,88 @@ impl StringArrayBuilder {
}
}

pub(crate) struct LargeStringArrayBuilder {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to make the already existing StringArrayBuilder generic but was having issues 😢

Signed-off-by: Devan <[email protected]>
Signed-off-by: Devan <[email protected]>
Comment on lines 71 to 74
Ok(match &arg_types[0] {
LargeUtf8 => LargeUtf8,
_ => Utf8,
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currently Utf8View isn't declared as an expected return type,
but the code does return Utf8View values.
should anything be failing when the runtime value type doesn't match the declared return type?
(if so, what would be a test case exercising such case?)

cc @comphead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added test cases for Utf8View and LargeUtf8 return types. Have gone ahead and adjusted this.

block: String
}

impl StringViewArrayBuilder {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StringViewArrayBuilder works but it almost feels "hacky"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the original intent of StringViewBuilder when @JasonLi-cn added it was to have a way to append (but not complete) strings to avoid a copy and not incrementally update the null mask.

Specifically, StringBuilder in arrow https://docs.rs/arrow/latest/arrow/array/type.StringBuilder.html#method.append_value lets you append an entire string but it requires the input be contiguous bytes

StringArrayBuilder is that you can push bytes (via write) wthout copying the data.

The reason I think it feels hacky is that the API for building up single values in a StringArray or StringViewArray without a copy is not present

As we see we want to do the same thing with Utf8View I wonder if we should spend some time making the API eaiser to use 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I filed apache/arrow-rs#6347 to discuss adding an API like this in arrow as well

Signed-off-by: Devan <[email protected]>
Signed-off-by: Devan <[email protected]>
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @devanbenz @findepi and @tshauck

I think this PR is good enough to merge, but I agree with @devanbenz 's observation https://github.com/apache/datafusion/pull/12224/files#r1740156822 that StringViewArrayBuilder feels hacky.

Specifically I think what is happening is that StringArrayBuilder is for a pretty specialized usecase, and thus so is StringViewArrayBuilder

I filed apache/arrow-rs#6347 upstream with a description of the use case to see if we can come up with something more general

if data_type == &Utf8View {
dt = data_type;
}
if data_type == &LargeUtf8 && dt != &Utf8View {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

block: String
}

impl StringViewArrayBuilder {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the original intent of StringViewBuilder when @JasonLi-cn added it was to have a way to append (but not complete) strings to avoid a copy and not incrementally update the null mask.

Specifically, StringBuilder in arrow https://docs.rs/arrow/latest/arrow/array/type.StringBuilder.html#method.append_value lets you append an entire string but it requires the input be contiguous bytes

StringArrayBuilder is that you can push bytes (via write) wthout copying the data.

The reason I think it feels hacky is that the API for building up single values in a StringArray or StringViewArray without a copy is not present

As we see we want to do the same thing with Utf8View I wonder if we should spend some time making the API eaiser to use 🤔

block: String
}

impl StringViewArrayBuilder {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I filed apache/arrow-rs#6347 to discuss adding an API like this in arrow as well

@devanbenz
Copy link
Contributor Author

devanbenz commented Sep 2, 2024

Thank you @devanbenz @findepi and @tshauck

I think this PR is good enough to merge, but I agree with @devanbenz 's observation #12224 (files) that StringViewArrayBuilder feels hacky.

Specifically I think what is happening is that StringArrayBuilder is for a pretty specialized usecase, and thus so is StringViewArrayBuilder

I filed apache/arrow-rs#6347 upstream with a description of the use case to see if we can come up with something more general

Great, sounds good @alamb 🙏

@alamb alamb merged commit e4a9424 into apache:main Sep 3, 2024
24 checks passed
@alamb
Copy link
Contributor

alamb commented Sep 3, 2024

Thanks again @devanbenz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
functions sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update the CONCAT scalar function to support Utf8View
4 participants